Skip to content

Add data_ptr method to Vips::Image #418

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ankane
Copy link
Contributor

@ankane ankane commented Apr 11, 2025

Hi, this PR adds support for vips_image_get_data to Ruby with a new data_ptr method that returns a sized FFI::Pointer. This provides similar functionality as write_to_memory but without additional copying.

With the current implementation, it would be the user's responsibility to ensure the pointer has not been invalidated (but we could add a reference to the pointer to ensure the image is not garbage collected while the pointer is active - I'm not sure if there are other operations that invalidate the pointer).

Ref: FFI enum docs

@jcupitt
Copy link
Member

jcupitt commented Apr 12, 2025

Hi @ankane, thanks for this!

Unfortunately, vips_image_get_data() is not thread-safe -- it transforms the argument into a memory image, and if some other thread is using the same image for something else (images can become shared between threads by the operation cache), you'll see a crash.

https://www.libvips.org/API/current/libvips-header.html#vips-image-get-data

The threadsafe equivalent is vips_image_copy_memory(). This makes a new memory image from an existing image, and, if possible, does it with just a pointer-copy. This is exposed in ruby-vips as Image#copy_memory:

https://libvips.github.io/ruby-vips/Vips/Image.html#copy_memory-instance_method

https://www.libvips.org/API/current/VipsImage.html#vips-image-copy-memory

@ankane
Copy link
Contributor Author

ankane commented Apr 12, 2025

Thanks @jcupitt. It sounds like the thread-safe way to get a read-only pointer with minimal copying is to combine the two.

copy = image.copy_memory
read_ptr = copy.data_ptr

What do you think about having a read_ptr method for this (see updated code)?

@jcupitt
Copy link
Member

jcupitt commented Apr 12, 2025

Yes, that's better. Having unsafe_data_ptr in the public API makes me very uneasy -- it's likely to cause mysterious random crashes. Maybe just have read_ptr and hide the unsafe code in there?

What happens if the caller slices the result from read_ptr? Would the hidden reference to the underlying image be lost? It still sounds unsafe, though perhaps everyone who wants a FFI pointer will know that! Maybe a note in the docs stressing the danger would be useful too.

@ankane
Copy link
Contributor Author

ankane commented Apr 12, 2025

Sounds good. Calling ptr.slice seems to retain the reference (otherwise, adding GC.stress = true to the test case should fail).

@jcupitt
Copy link
Member

jcupitt commented Apr 13, 2025

Great! Please add a note to the changelog (and credit yourself!), and a note to the docs saying this is potentially a very expensive operation.

Thank you for doing this work!

@ankane
Copy link
Contributor Author

ankane commented Apr 13, 2025

Thanks @jcupitt. Re expensive: I feel like I'm missing something about how libvips works that may make this PR not very useful. Both write_to_memory and read_ptr produce the same result.

image = Vips::Image.new_from_file("image.png")
ptr = image.read_ptr
p image.write_to_memory == ptr.read_string(ptr.size) # => true

However, write_to_memory is cheaper (both with and without a previous operation).

require "benchmark/ips"
require "ruby-vips"

Benchmark.ips do |x|
  x.report("write_to_memory") do
    image = Vips::Image.new_from_file("image.png")
    image.write_to_memory
  end
  x.report("read_ptr") do
    image = Vips::Image.new_from_file("image.png")
    image.read_ptr
  end
end

Output

ruby 3.4.2 (2025-02-15 revision d2930f8e7a) +PRISM [arm64-darwin24]
Warming up --------------------------------------
     write_to_memory    52.000 i/100ms
            read_ptr     7.000 i/100ms
Calculating -------------------------------------
     write_to_memory    521.467 (± 4.4%) i/s    (1.92 ms/i) -      2.652k in   5.096587s
            read_ptr     75.914 (±26.3%) i/s   (13.17 ms/i) -    371.000 in   5.046573s

I'd expect them to be roughly the same when the image pixels aren't already loaded (since they produce the same result), and I'd expect read_ptr to be faster when the pixels are loaded (since it could return the pointer instead of copying). Do you know why this isn't the case?

@jcupitt
Copy link
Member

jcupitt commented Apr 14, 2025

You're right, I'd expect copy_memory and write_to_memory to be similar, but copy_memory is a lot slower. I'll have a look.

@jcupitt
Copy link
Member

jcupitt commented Apr 14, 2025

I made a small C prog for testing:

// compile with
//   gcc -g -Wall copywrite.c `pkg-config vips --cflags --libs`

#include <vips/vips.h>

int
main(int argc, char **argv)
{   
    VIPS_INIT(argv[0]);

    // 2.33s, 75mb
    for (int i = 0; i < 1000; i++) {
        VipsImage *image = vips_image_new_from_file(argv[1], NULL);
        size_t size = VIPS_IMAGE_SIZEOF_IMAGE(image);
        void *buf = g_try_malloc(size);
        VipsImage *x = vips_image_new_from_memory(buf, size,
            image->Xsize, image->Ysize, image->Bands, image->BandFmt);
        vips_image_write(image, x);
        g_object_unref(x);
        g_free(buf);
        g_object_unref(image);
    }

    // 2.93s, 75mb
    for (int i = 0; i < 1000; i++) {
        VipsImage *image = vips_image_new_from_file(argv[1], NULL);
        
        VipsImage *x = vips_image_new_memory();
        vips_image_write(image, x);
        g_object_unref(x);
        g_object_unref(image);
    }

    // 2.33s, 75mb
    for (int i = 0; i < 1000; i++) {
        VipsImage *image = vips_image_new_from_file(argv[1], NULL);
        void *buf = vips_image_write_to_memory(image, NULL);

        g_free(buf);
        g_object_unref(image);
    }
    
    return 0;
}

The middle one is what copy_memory does and is 2.93s (on this PC and with a 1450 x 2048 pixel JPG). The top one is the same process but doing the malloc ourselves, and runs 20% faster, which puzzles me.

Anyway, the difference is relatively small, and not the huge difference you are seeing, which means it must be an inefficiency in ruby-vips.

The same program in ruby-vips is:

#!/usr/bin/env ruby

require "ruby-vips"

1000.times do |i|
  puts "loop #{i} ..."

  image = Vips::Image.new_from_file(ARGV[0])
  # 4.0s, 200mb
  #image.write_to_memory

  # 18.9s, 2.1gb
  image.copy_memory
end

So copy_memory is incredibly slow and needs 10x the memory.

Looking at the code, it's just:

    def copy_memory
      new_image = Vips.vips_image_copy_memory self
      Vips::Image.new new_image
    end

Which (I think?) should be OK. I'll keep digging.

@jcupitt
Copy link
Member

jcupitt commented Apr 14, 2025

Changing copy_memory to be:

    def copy_memory
      new_image = Vips.vips_image_copy_memory self
      ::GObject.g_object_unref new_image
    end

Obviously won't work, haha, but it gets the time down to 3.2s.

If I make it:

    def copy_memory
      Vips::Image.new_from_memory self.write_to_memory, 
          self.width, self.height, self.bands, self.format
    end

I see 4.0s, 240mb, so the same as write_to_memory.

I'm not sure why Vips::Image.new is so slow, it's just wrapping a pointer. I don't suppose you have any thoughts?

@ankane
Copy link
Contributor Author

ankane commented Apr 14, 2025

Sampling the following program on Mac arm64:

require "ruby-vips"

loop do
  image = Vips::Image.new_from_file("image.png")
  image.copy_memory
end

shows it's spending most of the time (~95%) on vips_sink_memory in libvips. Out of ~2150 samples:

2049 ffi_call_SYSV  (in ffi_c.bundle) + 80
  2041 vips_image_copy_memory  (in libvips.42.dylib) + 164
  ! 2041 vips_image_write  (in libvips.42.dylib) + 100
  !   2015 vips_image_generate  (in libvips.42.dylib) + 420
  !   : 2015 vips_sink_memory  (in libvips.42.dylib) + 248
  !   :   1750 vips_threadpool_run  (in libvips.42.dylib) + 328

@jcupitt
Copy link
Member

jcupitt commented Apr 15, 2025

That's very strange. I'll try sysprof here.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants